fix: update ModelFactory test assertions for mock provider#2068
fix: update ModelFactory test assertions for mock provider#2068nick-inkeep merged 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a straightforward, correctly scoped fix that updates test assertions to match the current BUILT_IN_PROVIDERS list after PR #2056 added the mock provider.
🔴❗ Critical (0) ❗🔴
No critical issues identified.
🟠⚠️ Major (0) 🟠⚠️
No major issues identified.
🟡 Minor (0) 🟡
No minor issues identified.
💭 Consider (1) 💭
💭 1) ModelFactory.test.ts Use partial string matching for more resilient test assertions
Issue: The test assertions in agents-api use full string matching for error messages, which requires updates whenever the provider list changes.
Why: The agents-core tests use partial string matching ('Unsupported provider: unsupported') or regex (/Unsupported provider/), making them resilient to provider list additions. This PR demonstrates the maintenance burden of full string matching.
Fix: Consider updating to partial string matching in a follow-up PR:
// Instead of:
expect(() => ModelFactory.createModel(config)).toThrow(
'Unsupported provider: unsupported. Supported providers are: anthropic, azure, ...'
);
// Consider:
expect(() => ModelFactory.createModel(config)).toThrow(
'Unsupported provider: unsupported'
);
// or
expect(() => ModelFactory.createModel(config)).toThrow(/Unsupported provider: unsupported/);Refs:
- agents-core/model-factory.test.ts:88 — existing resilient pattern
- agents-core/mock-provider.test.ts:319 — regex pattern example
✅ APPROVE
Summary: Clean, correctly scoped fix. The changes accurately update 3 test assertions to match the current BUILT_IN_PROVIDERS list after the mock provider was added in PR #2056. No bugs, security issues, or AGENTS.md violations detected. Ship it! 🚀
Discarded (0)
No findings were discarded.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: The Consider item was identified during orchestrator synthesis based on codebase exploration showing different assertion patterns across packages.
…anges The mock provider PR (#2056) added 'mock' to BUILT_IN_PROVIDERS but didn't update hardcoded error message assertions in agents-api tests. - Switch 3 assertions to substring match on the stable prefix instead of hardcoding the full provider list (won't break on next addition) - Remove redundant unsupported-provider test from mock-provider.test.ts (already covered by dedicated ModelFactory tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
99c7f12 to
de1f191
Compare
Three targeted fixes to prevent catastrophic cache invalidation in CI: 1. Exclude test files from build task inputs - prevents test file changes in core packages from cascading build hash invalidation to all 11+ downstream packages. Uses officially documented $TURBO_DEFAULT$ with negation globs (turbo 1.12+). 2. Remove transit dependency from lint task - transit is a no-op coordination task (no package defines a transit script) but its hash changes on any file change, cascading to all downstream lint tasks. Lint only reads local source files and doesn't need dependency ordering. 3. Move TURBO_TOKEN/TURBO_TEAM to job-level env - ensures all turbo invocations (check, knip) use remote cache, not just pnpm check. Also adds timeout-minutes: 30 as a safety guardrail. Evidence: PR #2068 changed 2 test files but caused 36/45 cache misses. With these fixes, the same change would cause ~6 misses (only the directly affected packages). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…verhead (#2076) * chore: improve CI performance by upgrading runner and removing test overhead - Upgrade ci job runner from ubuntu-latest to ubuntu-16gb for more resources - Remove OpenTelemetry NodeSDK initialization from test setup (was creating full auto-instrumentation per worker thread with no benefit in unit tests) - Reduce agents-api vitest maxThreads from 10 to 8 and minThreads from 4 to 2 to better match runner core count and reduce per-worker initialization cost Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove unused OTel test devDependencies Remove @opentelemetry/exporter-trace-otlp-proto and @opentelemetry/sdk-metrics from agents-api devDependencies since they were only used in the test setup OTel initialization that was removed in the previous commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf: fix turbo cache cascade invalidation Three targeted fixes to prevent catastrophic cache invalidation in CI: 1. Exclude test files from build task inputs - prevents test file changes in core packages from cascading build hash invalidation to all 11+ downstream packages. Uses officially documented $TURBO_DEFAULT$ with negation globs (turbo 1.12+). 2. Remove transit dependency from lint task - transit is a no-op coordination task (no package defines a transit script) but its hash changes on any file change, cascading to all downstream lint tasks. Lint only reads local source files and doesn't need dependency ordering. 3. Move TURBO_TOKEN/TURBO_TEAM to job-level env - ensures all turbo invocations (check, knip) use remote cache, not just pnpm check. Also adds timeout-minutes: 30 as a safety guardrail. Evidence: PR #2068 changed 2 test files but caused 36/45 cache misses. With these fixes, the same change would cause ~6 misses (only the directly affected packages). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: auto-format with biome --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* chore: improve CI performance by upgrading runner and removing test overhead - Upgrade ci job runner from ubuntu-latest to ubuntu-16gb for more resources - Remove OpenTelemetry NodeSDK initialization from test setup (was creating full auto-instrumentation per worker thread with no benefit in unit tests) - Reduce agents-api vitest maxThreads from 10 to 8 and minThreads from 4 to 2 to better match runner core count and reduce per-worker initialization cost Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove unused OTel test devDependencies Remove @opentelemetry/exporter-trace-otlp-proto and @opentelemetry/sdk-metrics from agents-api devDependencies since they were only used in the test setup OTel initialization that was removed in the previous commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf: fix turbo cache cascade invalidation Three targeted fixes to prevent catastrophic cache invalidation in CI: 1. Exclude test files from build task inputs - prevents test file changes in core packages from cascading build hash invalidation to all 11+ downstream packages. Uses officially documented $TURBO_DEFAULT$ with negation globs (turbo 1.12+). 2. Remove transit dependency from lint task - transit is a no-op coordination task (no package defines a transit script) but its hash changes on any file change, cascading to all downstream lint tasks. Lint only reads local source files and doesn't need dependency ordering. 3. Move TURBO_TOKEN/TURBO_TEAM to job-level env - ensures all turbo invocations (check, knip) use remote cache, not just pnpm check. Also adds timeout-minutes: 30 as a safety guardrail. Evidence: PR #2068 changed 2 test files but caused 36/45 cache misses. With these fixes, the same change would cause ~6 misses (only the directly affected packages). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: auto-format with biome * fix: add ENVIRONMENT to CORS test mocks broken by #2066 Commit 37e72ed gated localhost CORS on env.ENVIRONMENT but did not update the test mocks, causing 4 CORS tests to fail in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: add explicit permissions block to CI workflow Matches the pattern used by release.yml, ci-maintenance.yml, and stale.yml. Documents intent and prevents unintended privilege escalation if the workflow is later modified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
agents-apiModelFactory.test.tsto includemockin the supported providers listmocktoBUILT_IN_PROVIDERSinagents-corebut didn't update the corresponding test assertions inagents-api, breaking CI on all subsequent PRsTest plan
🤖 Generated with Claude Code